Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(container): experimental cloudbuilder support #5928

Merged
merged 13 commits into from
Apr 16, 2024
Merged

Conversation

stefreak
Copy link
Member

@stefreak stefreak commented Apr 11, 2024

What this PR does / why we need it:
Experimental cloudbuilder support for alpha preview users.

Which issue(s) this PR fixes:

Implements Garden Cloud Builder support, but also Fixes #5437

Special notes for your reviewer:
Tests are missing. I'd like to start dogfooding and validating this, and then implement tests before we make it generally available.

@stefreak stefreak requested a review from thsig April 11, 2024 15:41
Copy link
Collaborator

@thsig thsig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great stuff! Just commenting for how since this is still marked as a draft.

Experimental cloudbuilder support for test customers.
@stefreak stefreak marked this pull request as ready for review April 12, 2024 10:18
@stefreak stefreak requested a review from thsig April 12, 2024 10:18
thsig
thsig previously approved these changes Apr 12, 2024
thsig
thsig previously approved these changes Apr 12, 2024
@stefreak stefreak force-pushed the cloudbuilder-v1 branch 2 times, most recently from 32d8371 to 083fc92 Compare April 12, 2024 13:00
@stefreak stefreak requested review from edvald and thsig April 12, 2024 17:33
@stefreak stefreak requested a review from vvagaytsev April 15, 2024 11:55
Comment on lines 54 to 56
proc.on("exit", () => {
resolve()
})
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we simplify this to be just proc.on("exit", resolve) like in the line above?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can do that, but that'd change the behaviour; Right now the Promise resolves to void and in your variant it'd resolve to whatever the exit event handler receives as first argument (Likely the exit code). I only added the error here as I noticed that in case of an error the Promise would never be resolved or rejected. The error event will pass the error itself to reject as first argument.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, right! Thanks for the explanation :)

Copy link
Collaborator

@vvagaytsev vvagaytsev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! 💯 LGTM! 👍
I left some comments, please ping me when it's ready for the re-review :)


let isCloudBuilderEnabled = containerProvider.config.gardenCloudBuilder?.enabled || false

// The env variable GARDEN_CLOUD_BUILDER can be used to override the cloudbuilder.enabled config setting.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The setting name seems to be gardenCloudBuilder.enabled.

})

// public API
export const cloudbuilder = {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's rename this to cloudBuilder.

@vvagaytsev
Copy link
Collaborator

Thank you! 👍 The comments above are not critical and can be addressed in a follow-up PR.

@stefreak stefreak added this pull request to the merge queue Apr 16, 2024
Merged via the queue into main with commit 3f28841 Apr 16, 2024
41 checks passed
@stefreak stefreak deleted the cloudbuilder-v1 branch April 16, 2024 09:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: Linux ARM build unusable: Command mavend doesn't have a spec for this platform/architecture (linux-arm64)
4 participants